Ephemeral MARF #6365
Ephemeral MARF #6365
Conversation
…e on disk, and a RAM-backed MARF store which loads data from a read-only handle. Implement ClarityBlockStore for the ephemeral variant
fix: correct ephemeral open-tip height
…ich combine to form a WritableMarfStore trait. This factoring consolidates code between PersistentWritableMarfStore and EphemeralMarfStore and separates transaction lifecycle concerns from the act of writing data.
…-core into feat/shadow-marf
| /// continue to build on it and the network can service RPC requests on its state. | ||
| /// | ||
| /// These needs are each captured in distinct methods for committing this transaction. | ||
| pub trait ClarityMarfStoreTransaction { |
There was a problem hiding this comment.
I think the new trait should go in clarity_vm::clarity, rather than here.
| /// of giving the Clarity VM a backing store. Writes will be stored to the ephemeral MARF, and | ||
| /// reads will be carried out against either the ephemeral MARF or the read-only MARF, depending on | ||
| /// whether or not the opened chain tip refers to a block in the former or the latter. | ||
| pub struct EphemeralMarfStore<'a> { |
There was a problem hiding this comment.
Maybe a new module database::ephemeral?
| @@ -35,6 +55,7 @@ use crate::util_lib::db::{Error as DatabaseError, IndexDBConn}; | |||
| pub struct MarfedKV { | |||
| chain_tip: StacksBlockId, | |||
| marf: MARF<StacksBlockId>, | |||
| ephemeral_marf: Option<MARF<StacksBlockId>>, | |||
There was a problem hiding this comment.
It's kind of unfortunate we need this field to exist (I think its only here so that there's an owner for the ephemeral MARF which is opened for an ephemeral MARF transaction?), but I can't think of an easy way to refactor things so that its not necessary.
Can you add a rustdoc for this field that explains that it is only ever used to pin/keep ownership for an ephemeral MARF transaction?
It might also be a good idea to clear this field in a Drop for EphemeralMarfStore impl.
There was a problem hiding this comment.
I think its only here so that there's an owner for the ephemeral MARF which is opened for an ephemeral MARF transaction?
Yup. It's unfortunate that Rust's type system isn't expressive enough to just place it into the EphemeralMarf struct. Happy to document why.
There was a problem hiding this comment.
Yes, please document this field.
| let tx = if let Some(ephemeral_marf) = self.ephemeral_marf.as_mut() { | ||
| // attach the disk-backed MARF to the ephemeral MARF | ||
| EphemeralMarfStore::attach_read_only_marf(&ephemeral_marf, &read_only_marf).map_err( | ||
| |e| { | ||
| InterpreterError::Expect(format!( | ||
| "Failed to attach read-only MARF to ephemeral MARF: {:?}", | ||
| &e | ||
| )) | ||
| }, | ||
| )?; | ||
|
|
||
| let mut tx = ephemeral_marf.begin_tx().map_err(|e| { | ||
| InterpreterError::Expect(format!("Failed to open ephemeral MARF tx: {:?}", &e)) | ||
| })?; | ||
| tx.begin(&StacksBlockId::sentinel(), ephemeral_next) | ||
| .map_err(|e| { | ||
| InterpreterError::Expect(format!( | ||
| "Failed to begin first ephemeral MARF block: {:?}", | ||
| &e | ||
| )) | ||
| })?; | ||
| tx | ||
| } else { | ||
| // unreachable since self.ephemeral_marf is already assigned | ||
| unreachable!(); | ||
| }; |
There was a problem hiding this comment.
I think you can use let Some(..) = .. construct here to avoid nesting.
let Some(ephemeral_marf) = self.ephemeral_marf.as_mut() else {
// unreachable since self.ephemeral_marf is already assigned
unreachable!();
};
...| } | ||
|
|
||
| fn set_block_hash(&mut self, bhh: StacksBlockId) -> InterpreterResult<StacksBlockId> { | ||
| <dyn WritableMarfStore as ClarityBackingStore>::set_block_hash(&mut **self, bhh) |
There was a problem hiding this comment.
All of the <dyn WritableMarfStore as X>:: turbofish patterns can (and should) be replaced with just the trait invocation and all the &mut **self statements replaced with self.deref_mut() (and &**self replaced with self.deref():
| <dyn WritableMarfStore as ClarityBackingStore>::set_block_hash(&mut **self, bhh) | |
| ClarityBackingStore::set_block_hash(self.deref_mut(), bhh) |
That's true for all the <X as Y>:: invocations in these various impl definitions, e.g.,
<Self as ClarityMarfStoreTransaction>::should just be
ClarityMarfStoreTransaction::There was a problem hiding this comment.
I had considered that, but I wanted to be very explicit about the type conversion. Either one is fine with me though, as long as the right code gets generated.
There was a problem hiding this comment.
But there's no type conversion here, rather, its dynamic dispatch and this code is just specifying which trait method is being invoked (e.g., ClarityBackingStore). The deref() invocations are also more explicit (and readable) than &** syntax.
Description
Implements #6350 by re-implementing
WritableMarfStoreas an enum of a persistent disk-backed variant (the one we currently have), and a new ephemeral variant in which writes are committed to a RAM-backed Sqlite store, and in which reads will hit both the in-RAM sqlite store and an internalReadOnlyMarfStorehandle. This feature will be used to allow the node to replay blocks without the possibility of committing them to disk.Applicable issues
Additional info (benefits, drawbacks, caveats)
A caveat: blocks produced with the ephemeral variant will not have the same state root hash as they would have had they been produced with the persistent variant. This is a consequence of the fact that the new block's MARF key/value pairs and side-storage live in a separate MARF, whose trie is not linked to the disk-backed parent trie. This is acceptable because the purpose of this PR is to allow the node to replay already-processed blocks in order to extract the transaction receipts. The node already knows the "true" state root hash.
This PR is a draft until it has enough test coverage.
Checklist
docs/rpc/openapi.yamlandrpc-endpoints.mdfor v2 endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepobitcoin-tests.yml